Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add step by step nav GA4 tracking #3052

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Add step by step nav GA4 tracking #3052

merged 1 commit into from
Dec 1, 2022

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Nov 3, 2022 •
edited
Loading

Hi @andysellick & @JamesCGDS - would you be able to approve this if everything is OK?

Thanks 👍

What

  • Adds GTM tracking to the step by step nav component. It uses the same GTM object as the accordion tracking but with type: step by step instead.
  • It is enabled by adding ga4_tracking: true when the component is rendered on a page.
  • When ga4_tracking: true exists, it adds data-module="ga4-event-tracker" data-ga4-expandable. When this module is initialised, any child element that has data-ga4-event on it gets tracked on click. Therefore we add data-ga4-event where needed, which holds the JSON data we want to send to GTM on click. data-ga4-expandable is also added as this signals the ga4-event-tracker to track the value of aria-expanded and add it to the GTM object as 'opened'/'closed'.
  • The Javascript detects the ga4-event-tracker module, and populates the GTM JSONs for the 'Show all steps' button, and all the step buttons.

Why

  • This was meant to be added as part of the accordion tracking but we didn't have any confirmation on what the GTM Object should look like. As the step by step nav is very similar to the accordion component, we have used the same GTM object but with type: "step by step" instead.

Visual Changes

None.

@AshGDS AshGDS self-assigned this Nov 3, 2022
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 3, 2022 16:29 Inactive
andysellick
andysellick reviewed Nov 8, 2022
View reviewed changes
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, would definitely echo the comment that this needs some tests. I think a rb test to check the right attributes are applied in the right places, and a js test to check that the right things happen with the addition of attributes to the show/hide all button.

CHANGELOG.md Outdated Show resolved Hide resolved
app/views/govuk_publishing_components/components/_step_by_step_nav.html.erb Outdated Show resolved Hide resolved
app/views/govuk_publishing_components/components/docs/step_by_step_nav.yml Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 9, 2022 15:26 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from d36537a to c44ff1a Compare November 9, 2022 15:34
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 9, 2022 15:34 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from c44ff1a to bf3c85c Compare November 9, 2022 15:35
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 9, 2022 15:35 Inactive
@AshGDS
Copy link
Contributor Author

AshGDS commented Nov 9, 2022

Thanks @andysellick @JamesCGDS - I've added a JS test and a Ruby test. I'll squash the commits once everything is approved.

@andysellick we don't have ruby tests on the accordions/tabs/header etc. If we're adding a ruby test here, we should add them on the other components too in my opinion.

AshGDS
AshGDS commented Nov 9, 2022
View reviewed changes
spec/javascripts/components/step-by-step-nav-spec.js
@@ -4,9 +4,9 @@
describe('A stepnav module', function () {
'use strict'

var $element
var $element, element
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using element here as I'm not using jQuery like the other tests, and variables starting with $ indicate they're a jQuery variable, so I didn't use it.

@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from bf3c85c to eeb35e4 Compare November 10, 2022 11:46
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 10, 2022 11:47 Inactive
JamesCGDS
JamesCGDS approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@JamesCGDS JamesCGDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 Would it be possible to hold off on merging this until #3057 has been merged?

@AshGDS AshGDS changed the title Add step by step nav GA4 tracking [DO NOT MERGE] Add step by step nav GA4 tracking Nov 11, 2022
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from eeb35e4 to cfbdf25 Compare November 24, 2022 10:40
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 24, 2022 10:40 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from cfbdf25 to 9916830 Compare November 24, 2022 10:42
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 24, 2022 10:42 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from 9916830 to a0cef1f Compare November 24, 2022 14:28
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 24, 2022 14:28 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from a0cef1f to b8bd81b Compare November 24, 2022 14:32
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 24, 2022 14:32 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from b8bd81b to a24ccd2 Compare November 30, 2022 11:37
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 11:38 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from a24ccd2 to dc70c88 Compare November 30, 2022 11:42
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 11:42 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from dc70c88 to a9b02f3 Compare November 30, 2022 11:57
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 11:58 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from a9b02f3 to e13d7d6 Compare November 30, 2022 12:12
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 12:13 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from e13d7d6 to 4edff50 Compare November 30, 2022 12:25
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 12:25 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from 4edff50 to 421c44f Compare November 30, 2022 12:33
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 12:34 Inactive
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from 421c44f to 345e6d0 Compare November 30, 2022 12:41
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 November 30, 2022 12:42 Inactive
@AshGDS
Copy link
Contributor Author

AshGDS commented Nov 30, 2022 •
edited
Loading

Hi @andysellick - the implementation has changed since James reviewed this. Due to the outcomes of our accordion refactor, I've refactored this implementation. Now the GTM JSON for everything is created using JavaScript. There was no use in hardcoding them onto the .erb.html file, as the JavaScript reconstructs the HTML anyway.

I haven't shared the function which adds the GTM JSON to the steps which is similar to the accordion function. (See line 86 in step-by-step-nav.js and line 61 in accordion.js.) This is because ga4-core.js doesn't initialise in the govuk_publishing_components repo, so the components will break in our developer frontend if the functions are shared. I think the functions are simple enough that having them duplicated isn't a massive issue.

Would you be able to review the code? Thanks 👍

andysellick
andysellick reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few nitpicks but nothing serious.

app/assets/javascripts/govuk_publishing_components/components/step-by-step-nav.js Outdated Show resolved Hide resolved
app/assets/javascripts/govuk_publishing_components/components/step-by-step-nav.js Outdated Show resolved Hide resolved
spec/javascripts/components/step-by-step-nav-spec.js Show resolved Hide resolved
spec/javascripts/components/step-by-step-nav-spec.js Outdated Show resolved Hide resolved
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from 345e6d0 to f29daf1 Compare December 1, 2022 11:07
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 December 1, 2022 11:07 Inactive
andysellick
andysellick approved these changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, a couple more nitpicks. But otherwise good 👍

app/assets/javascripts/govuk_publishing_components/components/step-by-step-nav.js Outdated
}
}

var ga4Data = this.$module.isGa4Enabled ? "data-ga4-event='" + JSON.stringify(ga4JSON) + "'" : '' // Construct GA4 data-attributes for ga4-event-tracker.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick - I'd be tempted to move this line inside the if statement above for clarity i.e.

var ga4Data = ''
// Construct GA4 data-attributes for ga4-event-tracker
if (this.$module.isGa4Enabled) {
  var ga4JSON = {
    event_name: 'select_content',
    type: 'step by step',
    text: titleText.trim(),
    index: i + 1,
    index_total: this.$module.totalSteps
  }
  ga4Data = "data-ga4-event='" + JSON.stringify(ga4JSON) + "'" : ''
}

app/assets/javascripts/govuk_publishing_components/components/step-by-step-nav.js Outdated
// if GA4 is enabled, set attributes on 'show all sections' for tracking using ga4-event-tracker
if (this.$module.isGa4Enabled) {
var indexTotal = this.$module.totalSteps
var showAllAttributesGa4 = { event_name: 'select_content', type: 'step by step', index: 0, index_total: indexTotal }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick - might as well put this.$module.totalSteps directly into the object rather than declaring it separately.

@AshGDS
Add Step by Step nav GA4 tracking
58276d3 
Enables GA4 tracking with the presence of 'ga4_tracking: true' on the component. ga4-event-tracker and data-ga4-expandable are then added to the template. We are then using JS to add all the GA4 JSONS. There is no use putting them on the HTML template, as the HTML gets reconstructed by JavaScript anyway in order to create the buttons.
@AshGDS AshGDS force-pushed the ga4-step-by-step-tracking branch from f29daf1 to 58276d3 Compare December 1, 2022 16:09
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3052 December 1, 2022 16:09 Inactive
@AshGDS AshGDS changed the title [DO NOT MERGE] Add step by step nav GA4 tracking Add step by step nav GA4 tracking Dec 1, 2022
@AshGDS AshGDS merged commit a39e7f9 into main Dec 1, 2022
@AshGDS AshGDS deleted the ga4-step-by-step-tracking branch December 1, 2022 16:35
@AshGDS AshGDS mentioned this pull request Dec 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesCGDS JamesCGDS JamesCGDS approved these changes

@andysellick andysellick andysellick approved these changes

Assignees

@AshGDS AshGDS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AshGDS @andysellick @JamesCGDS @govuk-ci